Run vulnerability scan on latest release tags #5148
Conversation
| go-version: 1.24.0 | ||
| # Always use the latest Go release to avoid false positives from older | ||
| # versions of the Go standard library | ||
| go-version: stable |
There was a problem hiding this comment.
Shouldn't we run with the same Go version that the rest of the repository uses? To know if we need to update Go to address vulnerabilities.
There was a problem hiding this comment.
I'll support @denyeart . Go should be the one you build and test with.
There was a problem hiding this comment.
Since Fabric is publishing binaries rather than being consumed as a library, that is a good point. However, that makes me think that the way Fabric is handling Go versions is wrong. Using the Go version currently being used to build Fabric doesn't help identify standard library issues when scanning previously released versions. Probably the go.mod file should be specifying an explicit toolchain version. This then allows the correct Go version for a given release to be used when retrospectively scanning for vulnerabilities. It also removes the need to keep updating specific Go versions across all the GitHub Actions workflows. They can just use stable and the Go toolchain version will actually be used. What do you think?
There was a problem hiding this comment.
I'm not sure I'm on board yet.
From https://go.dev/doc/toolchain:
When the go or toolchain line is newer than the bundled toolchain, the go command runs the newer toolchain instead.
This tells me if the installed (bundled) version from the workflow is later than toolchain version, it will use installed (bundled) version. This seems to contradict your proposal of using stable for the workflows and a toolchain version in go.mod, that is, I don't think it will actually report vulnerabilities in prior release's toolchain as you were hoping.
Also:
"A go.mod that says go 1.21.0 with no toolchain line is interpreted as if it had a toolchain go1.21.0 line."
Because of this, we had decided to switch the go.mod go version to the three digit version, and not set the toolchain version at all. Just let the go version dictate the toolchain version implicitly (fewer moving parts to manage and less to think about). A matching workflow Go version and go.mod go version seemed the most clear and locked-down, with fewest considerations and edge case to think about.
In summary, I do see some value in your proposal to scan with prior release's Go version, however I'm not sure it actually will do it, or if it is compelling enough to change our current approach. I may have misinterpreted something however...
There was a problem hiding this comment.
Specifying the toolchain either with an explicit toolchain directive or using the go directive is fine. Keeping the version of Go used to run the Fabric build in step with the version in the go directive also seems fine to me. Unfortunately, the version in the go.mod file has not kept in step with the version used in the GitHub Actions workflows.
For libraries, I like keeping the go/toolchain directive in the go.mod file to the lowest supported Go version (including patch level) since this imposes fewest restrictions on consumers, who are then free to use a more up-to-date Go version to avoid standard library vulnerabilities. For a binary, it might make more sense for the go/toolchain directive to match the version of Go used to build.
|
I don't like that you put everything in one action. |
This PR doesn't introduce or change that behaviour. It might be good to capture this requirement in an issue so it isn't forgotten. |
bestbeforetoday
force-pushed
the
scan-releases
branch
from
2a6e4ed to
908805b
Compare
|
I have updated this change to:
|
bestbeforetoday
force-pushed
the
scan-releases
branch
from
908805b to
fa9b7fc
Compare
| go-version: 1.24.0 | ||
| go-version: stable |
There was a problem hiding this comment.
We have everywhere the version of go in go.mod matches the version of go we build images with and test them with.
Your last change may lead to vulnerability checking with a version of go that fabric doesn't support yet.
There was a problem hiding this comment.
We have everywhere the version of go in go.mod matches the version of go we build images with and test them with.
v3.0.0 release matches:
- .github/workflows/release.yml says
1.23.1. - go.mod says
1.23.1.
v2.5.11 release does not match:
- .github/workflows/release.yml says
1.23.5 - go.mod says
1.23.1.
Go 1.23.1 contains standard library vulnerabilities that are used by Fabric, so v3.0.0 contains vulnerabilities our current scanning is blissfully unaware of.
Your last change may lead to vulnerability checking with a version of go that fabric doesn't support yet.
The version of Go in the workflow is used only to run the osv-scanner tool. It has no relation to the version of Go used by Fabric.
There was a problem hiding this comment.
As a developer, I'm interested in the vulnerabilities that are currently in the last commit of the main branch. This check should be done with the version of go that is being tested, not the stable version.
There was a problem hiding this comment.
To repeat what I've said before, the version of Go used for the scan is the one specified as the toolchain in the go.mod file, not the one in this workflow file. I was trying to save you some work keeping versions updated by just using the latest Go version here since it makes no difference to the scan results. It is fine by me if you prefer to keep maintaining the version in this file, so I've updated the PR as you requested,
bestbeforetoday
force-pushed
the
scan-releases
branch
2 times, most recently
from
7ace3f7 to
c931c9e
Compare
Vulnerability scans were previously run only on the latest state of currently developed branches. This provided assurance that the current branch state did not contain known vulnerabilities in dependencies, but did not provide assurance that the currently released code was free of vulnerabilities. This change runs additional vulnerability scans on the most recent release version tag for currently developed branches. Scan failures now indicate that a new release is required to address vulnerabilities in dependencies. Signed-off-by: Mark S. Lewis <[email protected]>
bestbeforetoday
force-pushed
the
scan-releases
branch
from
c931c9e to
af3e4a5
Compare
|
@bestbeforetoday This is expected for v3.0.0, I'll plan to resolve by doing a v3.0.1 release. For v2.5.11 I think we are actually ok since the release builds actually used Go v1.23.5 (I had forgotten to update go.mod so it is still being scanned at Go v1.23.1). Going forward we will keep the Go versions in sync, and when we notice a last release turns red we can make sure dependencies are up to date and then cut the next patch release. Make sense? |
Vulnerability scans were previously run only on the latest state of currently developed branches. This provided assurance that the current branch state did not contain known vulnerabilities in dependencies, but did not provide assurance that the currently released code was free of vulnerabilities.
This change runs additional vulnerability scans on the most recent release version tag for currently developed branches. Scan failures now indicate that a new release is required to address vulnerabilities in dependencies.